Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ktlint performances when the input patterns are absolute paths #1144

Merged
merged 1 commit into from
Jun 15, 2021
Merged

Improve ktlint performances when the input patterns are absolute paths #1144

merged 1 commit into from
Jun 15, 2021

Conversation

rciovati
Copy link
Contributor

@rciovati rciovati commented May 8, 2021

Description

This PR proposes a solution for the performance issue described in #1135.

When ktlint gets a bunch of absolute paths there is no need to walk through the file system and search for them, we can just resolve and analyze them directly.

Before:

➜  android-listeners git:(rc/kitlint) ktlint -F --debug --relative MyFile.kt
[DEBUG] Discovered ruleset with "standard" id.
[DEBUG] Discovered reporter with "baseline" id.
[DEBUG] Discovered reporter with "checkstyle" id.
[DEBUG] Discovered reporter with "json" id.
[DEBUG] Discovered reporter with "html" id.
[DEBUG] Discovered reporter with "plain" id.
[DEBUG] Initializing "plain" reporter with {verbose=true, color=false, color_name=DARK_GRAY}
[DEBUG] Checking MyFile.kt
Loaded .editorconfig: [insert_final_newline: true, max_line_length: 200]
[DEBUG] 30291ms / 1 file(s) / 0 error(s)

After:

➜  android-listeners git:(rc/kitlint) java -jar ktlint-0.42.0-SNAPSHOT-all.jar -F --debug --relative MyFile.kt
[DEBUG] Discovered ruleset with "standard" id.
[DEBUG] Discovered reporter with "baseline" id.
[DEBUG] Discovered reporter with "checkstyle" id.
[DEBUG] Discovered reporter with "json" id.
[DEBUG] Discovered reporter with "html" id.
[DEBUG] Discovered reporter with "plain" id.
[DEBUG] Initializing "plain" reporter with {verbose=true, color=false, color_name=DARK_GRAY}
[DEBUG] Checking MyFile.kt
Loaded .editorconfig: [insert_final_newline: true, max_line_length: 200]
[DEBUG] 847ms / 1 file(s) / 0 error(s)

In our project it speeds up the execution from ~30s down to less than 1s.

No additional tests were added, the exist ones passes.

Checklist

  • tests are added
  • CHANGELOG.md is updated

@rciovati
Copy link
Contributor Author

it looks like the build fails because of a dependency verification issue which I believe is not related to my change but I'll see what I can do.

For more details see https://docs.gradle.org/6.8.3/release-notes.html

Starting a Gradle Daemon (subsequent builds will be faster)
Dependency verification is an incubating feature.
> Task :buildSrc:extractPrecompiledScriptPluginPlugins
> Task :buildSrc:generateExternalPluginSpecBuilders FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':buildSrc:generateExternalPluginSpecBuilders'.
> Dependency verification failed for configuration ':buildSrc:compileClasspath'
  2 artifacts failed verification:
    - dokka-gradle-plugin-0.10.1.jar (org.jetbrains.dokka:dokka-gradle-plugin:0.10.1) from repository MavenRepo
    - dokka-gradle-plugin-0.10.1.pom (org.jetbrains.dokka:dokka-gradle-plugin:0.10.1) from repository MavenRepo
  If the artifacts are trustworthy, you will need to update the gradle/verification-metadata.xml file by following the instructions at https://docs.gradle.org/6.8.3/userguide/dependency_verification.html#sec:troubleshooting-verification
  
  Open this report for more details: file:///home/runner/work/ktlint/ktlint/build/reports/dependency-verification/at-1620670437192/dependency-verification-report.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 49s
Error: Process completed with exit code 1.

@rciovati rciovati changed the title Do not traverse the file system when the input patterns are absolute paths Improve ktlint performances when the input patterns are absolute paths May 14, 2021
@shashachu
Copy link
Contributor

@rciovati thanks so much for this change. I just merged a PR which should fix the CI builds; can you rebase?

@rciovati
Copy link
Contributor Author

@shashachu rebased, thanks!

@rciovati rciovati requested a review from shashachu May 25, 2021 14:48
@shashachu
Copy link
Contributor

argh. windows build failing. 🤦‍♀️

@shashachu
Copy link
Contributor

@rciovati do you have this commit rebased in? fae2362

It might be the same issue.

@rciovati
Copy link
Contributor Author

@shashachu I've just rebased one more time!

@shashachu
Copy link
Contributor

haha it's killing me! i'm not sure what's happening with the dependency verification...

@Tapchicoma
Copy link
Collaborator

@rciovati could you rebase again? This time verification should pass

@rciovati rciovati requested a review from shashachu May 28, 2021 17:55
@Tapchicoma
Copy link
Collaborator

@rciovati tests are failing, could you update your change, so they will pass?

@rciovati
Copy link
Contributor Author

@Tapchicoma I'll try. I don't have a windows machine so this might take a while.

@shashachu
Copy link
Contributor

@rciovati @Tapchicoma Path.resolve seems to throw an exception on Windows when you pass in file globs. This change makes the tests pass; does it seem valid or too hacky?

val (existingFiles, actualGlobs) = globs.partition {
        try {
            Files.isRegularFile(rootDir.resolve(it))
        } catch (e: InvalidPathException) {
            false
        }
    }

@rciovati
Copy link
Contributor Author

@shashachu it looks valid to me! I've just pushed the change. Thank you so much for looking into this one 🙏

@shashachu
Copy link
Contributor

@rciovati looks like you are missing an import :) e: /home/runner/work/ktlint/ktlint/ktlint/src/main/kotlin/com/pinterest/ktlint/internal/FileUtils.kt: (31, 21): Unresolved reference: InvalidPathException

@rciovati
Copy link
Contributor Author

@shashachu of course 🤦 fixed

@shashachu shashachu merged commit a945cb5 into pinterest:master Jun 15, 2021
@shashachu
Copy link
Contributor

hallelujah! thanks @rciovati for your patience!

@rciovati rciovati deleted the rc/paths branch June 17, 2021 09:17
romtsn pushed a commit to paul-dingemans/ktlint that referenced this pull request Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants